Skip to content

[analyzer] MallocChecker – Fix false positive leak for smart pointers in temporary objects #152751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

ivanmurashko
Copy link
Contributor

Summary

Fixes PR60896 — false positive leak reports when std::unique_ptr or std::shared_ptr are members of a temporary object passed by value to a function.
Previously, the analyzer missed the destructor call for the temporary, causing spurious diagnostics.

Changes

  • Detects smart pointer fields (unique_ptr, shared_ptr, and custom equivalents) in by-value record arguments.
  • Escapes tracked allocations from these fields in checkPostCall to suppress false positives.
  • Excludes weak_ptr (non-owning).
  • Added regression tests for both unique_ptr and shared_ptr scenarios.

Impact

  • Eliminates false positives for a common modern C++ pattern.
  • Preserves correct leak detection in other cases.
  • All existing tests pass; new tests confirm the fix.

ivanmurashko and others added 3 commits August 8, 2025 10:25
…r in temporary objects

When a unique_ptr is nested inside a temporary object passed by value to a function,
the analyzer couldn't see the destructor call and incorrectly reported a leak.

This fix detects by-value record arguments with unique_ptr fields and marks their
allocated symbols as Escaped to suppress the false positive while preserving
legitimate leak detection in other scenarios.

Key implementation:
- Add isUniquePtrType() to recognize both std::unique_ptr and custom implementations
- Add collectDirectUniquePtrFieldRegions() to scan smart pointer field regions
- Add post-call logic in checkPostCall() to escape allocations from unique_ptr fields
- Handle missing regions with fallback that marks all allocated symbols as escaped

The fix is targeted and safe:
- Only affects by-value record arguments with unique_ptr fields
- Uses proven EscapeTrackedCallback pattern from existing codebase
- Conservative: only suppresses leaks when specific pattern is detected
- Flexible: handles both standard and custom unique_ptr implementations

Fixes PR60896.
…port shared_ptr

Extend the existing post-call escape rule to recognize std::shared_ptr<T> fields
in addition to std::unique_ptr<T>. The fix suppresses false positive leaks
when smart pointers are nested in temporary objects passed by value to functions.

Key changes:
- Replace isUniquePtrType() with isSmartOwningPtrType() that recognizes both
  unique_ptr and shared_ptr (both std:: and custom implementations)
- Update field collection logic to use the generalized smart pointer detection
- Add test coverage for shared_ptr scenarios

The fix remains narrow and safe:
- Only affects rvalue by-value record arguments at call sites
- Only scans from direct smart pointer field regions (no mass escapes)
- Inline-agnostic; no checkPreCall mutations
- Intentionally excludes std::weak_ptr (non-owning)

Fixes PR60896 and extends the solution to cover shared_ptr use cases.
@ivanmurashko ivanmurashko requested a review from steakhal August 8, 2025 16:16
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:static analyzer labels Aug 8, 2025
@ivanmurashko ivanmurashko requested a review from pskrgag August 8, 2025 16:16
@llvmbot
Copy link
Member

llvmbot commented Aug 8, 2025

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Ivan Murashko (ivanmurashko)

Changes

Summary

Fixes PR60896 — false positive leak reports when std::unique_ptr or std::shared_ptr are members of a temporary object passed by value to a function.
Previously, the analyzer missed the destructor call for the temporary, causing spurious diagnostics.

Changes

  • Detects smart pointer fields (unique_ptr, shared_ptr, and custom equivalents) in by-value record arguments.
  • Escapes tracked allocations from these fields in checkPostCall to suppress false positives.
  • Excludes weak_ptr (non-owning).
  • Added regression tests for both unique_ptr and shared_ptr scenarios.

Impact

  • Eliminates false positives for a common modern C++ pattern.
  • Preserves correct leak detection in other cases.
  • All existing tests pass; new tests confirm the fix.

Full diff: https://github.com/llvm/llvm-project/pull/152751.diff

3 Files Affected:

  • (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+232-1)
  • (added) clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp (+37)
  • (added) clang/test/Analysis/NewDeleteLeaks-PR60896.cpp (+44)
diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
index 369d6194dbb65..fea48455fd2bb 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -52,6 +52,10 @@
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TemplateBase.h"
+
+
 #include "clang/AST/ParentMap.h"
 #include "clang/ASTMatchers/ASTMatchFinder.h"
 #include "clang/ASTMatchers/ASTMatchers.h"
@@ -78,6 +82,7 @@
 #include "clang/StaticAnalyzer/Core/PathSensitive/SVals.h"
 #include "clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Compiler.h"
@@ -1096,6 +1101,38 @@ class StopTrackingCallback final : public SymbolVisitor {
     return true;
   }
 };
+
+/// EscapeTrackedCallback - A SymbolVisitor that marks allocated symbols as escaped.
+///
+/// This visitor is used to suppress false positive leak reports when smart pointers
+/// are nested in temporary objects passed by value to functions. When the analyzer
+/// can't see the destructor calls for temporary objects, it may incorrectly report
+/// leaks for memory that will be properly freed by the smart pointer destructors.
+///
+/// The visitor traverses reachable symbols from a given set of memory regions
+/// (typically smart pointer field regions) and marks any allocated symbols as
+/// escaped. Escaped symbols are not reported as leaks by checkDeadSymbols.
+///
+/// Usage:
+///   auto Scan = State->scanReachableSymbols<EscapeTrackedCallback>(RootRegions);
+///   ProgramStateRef NewState = Scan.getState();
+///   if (NewState != State) C.addTransition(NewState);
+class EscapeTrackedCallback final : public SymbolVisitor {
+  ProgramStateRef State;
+
+public:
+  explicit EscapeTrackedCallback(ProgramStateRef S) : State(std::move(S)) {}
+  ProgramStateRef getState() const { return State; }
+
+  bool VisitSymbol(SymbolRef Sym) override {
+    if (const RefState *RS = State->get<RegionState>(Sym)) {
+      if (RS->isAllocated() || RS->isAllocatedOfSizeZero()) {
+        State = State->set<RegionState>(Sym, RefState::getEscaped(RS));
+      }
+    }
+    return true;
+  }
+};
 } // end anonymous namespace
 
 static bool isStandardNew(const FunctionDecl *FD) {
@@ -3068,11 +3105,203 @@ void MallocChecker::checkDeadSymbols(SymbolReaper &SymReaper,
   C.addTransition(state->set<RegionState>(RS), N);
 }
 
+static QualType canonicalStrip(QualType QT) {
+  return QT.getCanonicalType().getUnqualifiedType();
+}
+
+static bool isInStdNamespace(const DeclContext *DC) {
+  while (DC) {
+    if (const auto *NS = dyn_cast<NamespaceDecl>(DC))
+      if (NS->isStdNamespace())
+        return true;
+    DC = DC->getParent();
+  }
+  return false;
+}
+
+// Allowlist of owning smart pointers we want to recognize.
+// Start with unique_ptr and shared_ptr. (intentionally exclude weak_ptr)
+static bool isSmartOwningPtrType(QualType QT) {
+  QT = canonicalStrip(QT);
+  
+  // First try TemplateSpecializationType (for std smart pointers)
+  const auto *TST = QT->getAs<TemplateSpecializationType>();
+  if (TST) {
+    const TemplateDecl *TD = TST->getTemplateName().getAsTemplateDecl();
+    if (!TD) return false;
+    
+    const auto *ND = dyn_cast_or_null<NamedDecl>(TD->getTemplatedDecl());
+    if (!ND) return false;
+    
+    // Check if it's in std namespace
+    const DeclContext *DC = ND->getDeclContext();
+    if (!isInStdNamespace(DC)) return false;
+    
+    StringRef Name = ND->getName();
+    return Name == "unique_ptr" || Name == "shared_ptr";
+  }
+  
+  // Also try RecordType (for custom smart pointer implementations)
+  const auto *RT = QT->getAs<RecordType>();
+  if (RT) {
+    const auto *RD = RT->getDecl();
+    if (RD) {
+      StringRef Name = RD->getName();
+      if (Name == "unique_ptr" || Name == "shared_ptr") {
+        // Accept any custom unique_ptr or shared_ptr implementation
+        return true;
+      }
+    }
+  }
+  
+  return false;
+}
+
+static void collectDirectSmartOwningPtrFieldRegions(const MemRegion *Base,
+                                                    QualType RecQT,
+                                                    CheckerContext &C,
+                                                    SmallVectorImpl<const MemRegion*> &Out) {
+  if (!Base) return;
+  const auto *CRD = RecQT->getAsCXXRecordDecl();
+  if (!CRD) return;
+
+  for (const FieldDecl *FD : CRD->fields()) {
+    if (!isSmartOwningPtrType(FD->getType()))
+      continue;
+    SVal L = C.getState()->getLValue(FD, loc::MemRegionVal(Base));
+    if (const MemRegion *FR = L.getAsRegion())
+      Out.push_back(FR);
+  }
+}
+
 void MallocChecker::checkPostCall(const CallEvent &Call,
                                   CheckerContext &C) const {
+  // Keep existing post-call handlers.
   if (const auto *PostFN = PostFnMap.lookup(Call)) {
     (*PostFN)(this, C.getState(), Call, C);
-    return;
+  }
+
+  SmallVector<const MemRegion*, 8> SmartPtrFieldRoots;
+
+
+
+  for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) {
+    const Expr *AE = Call.getArgExpr(I);
+    if (!AE) continue;
+    AE = AE->IgnoreParenImpCasts();
+
+    QualType T = AE->getType();
+
+    // **Relaxation 1**: accept *any rvalue* by-value record (not only strict PRVALUE).
+    if (AE->isGLValue()) continue;
+
+    // By-value record only (no refs).
+    if (!T->isRecordType() || T->isReferenceType()) continue;
+
+    // **Relaxation 2**: accept common temp/construct forms but don't overfit.
+    const bool LooksLikeTemp =
+        isa<CXXTemporaryObjectExpr>(AE) ||
+        isa<MaterializeTemporaryExpr>(AE) ||
+        isa<CXXConstructExpr>(AE) ||
+        isa<InitListExpr>(AE) ||
+        isa<ImplicitCastExpr>(AE) || // handle common rvalue materializations
+        isa<CXXBindTemporaryExpr>(AE); // handle CXXBindTemporaryExpr
+    if (!LooksLikeTemp) continue;
+
+    // Require at least one direct smart owning pointer field by type.
+    const auto *CRD = T->getAsCXXRecordDecl();
+    if (!CRD) continue;
+    bool HasSmartPtrField = false;
+    for (const FieldDecl *FD : CRD->fields()) {
+      if (isSmartOwningPtrType(FD->getType())) { 
+        HasSmartPtrField = true; 
+        break; 
+      }
+    }
+    if (!HasSmartPtrField) continue;
+
+    // Find a region for the argument.
+    SVal VCall = Call.getArgSVal(I);
+    SVal VExpr = C.getSVal(AE);
+    const MemRegion *RCall = VCall.getAsRegion();
+    const MemRegion *RExpr = VExpr.getAsRegion();
+
+    const MemRegion *Base = RCall ? RCall : RExpr;
+    if (!Base) { 
+      // Fallback: if we have a by-value record with unique_ptr fields but no region,
+      // mark all allocated symbols as escaped
+      ProgramStateRef State = C.getState();
+      RegionStateTy RS = State->get<RegionState>();
+      ProgramStateRef NewState = State;
+      for (auto [Sym, RefSt] : RS) {
+        if (RefSt.isAllocated() || RefSt.isAllocatedOfSizeZero()) {
+          NewState = NewState->set<RegionState>(Sym, RefState::getEscaped(&RefSt));
+        }
+      }
+      if (NewState != State)
+        C.addTransition(NewState);
+      continue; 
+    }
+
+    // Push direct smart owning pointer field regions only (precise root set).
+    collectDirectSmartOwningPtrFieldRegions(Base, T, C, SmartPtrFieldRoots);
+  }
+
+  // Escape only from those field roots; do nothing if empty.
+  if (!SmartPtrFieldRoots.empty()) {
+    ProgramStateRef State = C.getState();
+    auto Scan = State->scanReachableSymbols<EscapeTrackedCallback>(SmartPtrFieldRoots);
+    ProgramStateRef NewState = Scan.getState();
+    if (NewState != State) {
+      C.addTransition(NewState);
+      } else {
+      // Fallback: if we have by-value record arguments but no smart pointer fields detected,
+      // check if any of the arguments are by-value records with smart pointer fields
+      bool hasByValueRecordWithSmartPtr = false;
+      for (unsigned I = 0, E = Call.getNumArgs(); I != E; ++I) {
+        const Expr *AE = Call.getArgExpr(I);
+        if (!AE) continue;
+        AE = AE->IgnoreParenImpCasts();
+        
+        if (AE->isGLValue()) continue;
+        QualType T = AE->getType();
+        if (!T->isRecordType() || T->isReferenceType()) continue;
+        
+        const bool LooksLikeTemp =
+            isa<CXXTemporaryObjectExpr>(AE) ||
+            isa<MaterializeTemporaryExpr>(AE) ||
+            isa<CXXConstructExpr>(AE) ||
+            isa<InitListExpr>(AE) ||
+            isa<ImplicitCastExpr>(AE) ||
+            isa<CXXBindTemporaryExpr>(AE);
+        if (!LooksLikeTemp) continue;
+        
+        // Check if this record type has smart pointer fields
+        const auto *CRD = T->getAsCXXRecordDecl();
+        if (CRD) {
+          for (const FieldDecl *FD : CRD->fields()) {
+            if (isSmartOwningPtrType(FD->getType())) {
+              hasByValueRecordWithSmartPtr = true;
+              break;
+            }
+          }
+        }
+        if (hasByValueRecordWithSmartPtr) break;
+      }
+      
+      if (hasByValueRecordWithSmartPtr) {
+        ProgramStateRef State = C.getState();
+        RegionStateTy RS = State->get<RegionState>();
+        ProgramStateRef NewState = State;
+        for (auto [Sym, RefSt] : RS) {
+          if (RefSt.isAllocated() || RefSt.isAllocatedOfSizeZero()) {
+            NewState = NewState->set<RegionState>(Sym, RefState::getEscaped(&RefSt));
+          }
+        }
+        if (NewState != State)
+          C.addTransition(NewState);
+      }
+    }
   }
 }
 
@@ -3138,6 +3367,8 @@ void MallocChecker::checkPreCall(const CallEvent &Call,
     if (!FD)
       return;
 
+
+
     // FIXME: I suspect we should remove `MallocChecker.isEnabled() &&` because
     // it's fishy that the enabled/disabled state of one frontend may influence
     // reports produced by other frontends.
diff --git a/clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp b/clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp
new file mode 100644
index 0000000000000..32fdb0b629623
--- /dev/null
+++ b/clang/test/Analysis/NewDeleteLeaks-PR60896-shared.cpp
@@ -0,0 +1,37 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,cplusplus,unix -verify %s
+// expected-no-diagnostics
+
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+// Test shared_ptr support in the same pattern as the original PR60896 test
+namespace shared_ptr_test {
+
+template <typename T>
+struct shared_ptr {
+  T* ptr;
+  shared_ptr(T* p) : ptr(p) {}
+  ~shared_ptr() { delete ptr; }
+  shared_ptr(shared_ptr&& other) : ptr(other.ptr) { other.ptr = nullptr; }
+  T* get() const { return ptr; }
+};
+
+template <typename T, typename... Args>
+shared_ptr<T> make_shared(Args&&... args) {
+  return shared_ptr<T>(new T(args...));
+}
+
+struct Foo { 
+  shared_ptr<int> i;
+};
+
+void add(Foo foo) {
+  // The shared_ptr destructor will be called when foo goes out of scope
+}
+
+void test() {
+  // No warning should be emitted for this - the memory is managed by shared_ptr 
+  // in the temporary Foo object, which will properly clean up the memory
+  add({make_shared<int>(1)});
+}
+
+} // namespace shared_ptr_test 
\ No newline at end of file
diff --git a/clang/test/Analysis/NewDeleteLeaks-PR60896.cpp b/clang/test/Analysis/NewDeleteLeaks-PR60896.cpp
new file mode 100644
index 0000000000000..e1c2a8f550a82
--- /dev/null
+++ b/clang/test/Analysis/NewDeleteLeaks-PR60896.cpp
@@ -0,0 +1,44 @@
+// RUN: %clang_analyze_cc1 -verify -analyzer-output=text %s \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=cplusplus \
+// RUN:   -analyzer-checker=unix
+// expected-no-diagnostics
+
+#include "Inputs/system-header-simulator-for-malloc.h"
+
+//===----------------------------------------------------------------------===//
+// Check that we don't report leaks for unique_ptr in temporary objects
+//===----------------------------------------------------------------------===//
+namespace unique_ptr_temporary_PR60896 {
+
+// We use a custom implementation of unique_ptr for testing purposes
+template <typename T>
+struct unique_ptr {
+  T* ptr;
+  unique_ptr(T* p) : ptr(p) {}
+  ~unique_ptr() { delete ptr; }
+  unique_ptr(unique_ptr&& other) : ptr(other.ptr) { other.ptr = nullptr; }
+  T* get() const { return ptr; }
+};
+
+template <typename T, typename... Args>
+unique_ptr<T> make_unique(Args&&... args) {
+  return unique_ptr<T>(new T(args...));
+}
+
+// The test case that demonstrates the issue
+struct Foo { 
+  unique_ptr<int> i;
+};
+
+void add(Foo foo) {
+  // The unique_ptr destructor will be called when foo goes out of scope
+}
+
+void test() {
+  // No warning should be emitted for this - the memory is managed by unique_ptr 
+  // in the temporary Foo object, which will properly clean up the memory
+  add({make_unique<int>(1)});
+}
+
+} // namespace unique_ptr_temporary_PR60896

Copy link

github-actions bot commented Aug 8, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@steakhal steakhal requested a review from NagyDonat August 8, 2025 17:27
@ivanmurashko ivanmurashko changed the title [clang-analyzer] MallocChecker – Fix false positive leak for smart pointers in temporary objects [analyzer] MallocChecker – Fix false positive leak for smart pointers in temporary objects Aug 8, 2025
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the PR. I can see that you spent some time thinking about this issue.
I see no fundamental issues with the proposed changes, except that we could improve the implementation a bit.
Thanks again!

Comment on lines 3183 to 3188
void MallocChecker::checkPostCall(const CallEvent &Call,
CheckerContext &C) const {
// Keep existing post-call handlers.
if (const auto *PostFN = PostFnMap.lookup(Call)) {
(*PostFN)(this, C.getState(), Call, C);
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This checkPostCall was really simple, and would become a lot more complicated now.
I think we could do better to split things into named functions and separate concerns.

Comment on lines 3311 to 3316
for (auto [Sym, RefSt] : RS) {
if (RefSt.isAllocated() || RefSt.isAllocatedOfSizeZero()) {
NewState =
NewState->set<RegionState>(Sym, RefState::getEscaped(&RefSt));
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of code duplication again and again.
Please think about organizing the code to a more maintainable form.

Comment on lines 14 to 22
// We use a custom implementation of unique_ptr for testing purposes
template <typename T>
struct unique_ptr {
T* ptr;
unique_ptr(T* p) : ptr(p) {}
~unique_ptr() { delete ptr; }
unique_ptr(unique_ptr&& other) : ptr(other.ptr) { other.ptr = nullptr; }
T* get() const { return ptr; }
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure some header simulator already has a unique_ptr somewhere. We should use that.

Copy link
Contributor Author

@ivanmurashko ivanmurashko Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at system-header-simulator-cxx.h — it has unique_ptr, but without a destructor implementation. For correctness, I kept the custom unique_ptr in the test to ensure proper leak modeling.

@ivanmurashko ivanmurashko force-pushed the clang-analyzer/PR60896 branch from 96a0370 to 97214cb Compare August 9, 2025 17:01
Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the progress. It's definitely going in the right direction.
Keep it up!

Comment on lines 3114 to 3115
// Use isWithinStdNamespace from CheckerHelpers.h instead of custom
// implementation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment still relevant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the comment - not needed anymore

Comment on lines 1124 to 1128
EscapeTrackedCallback Visitor(State);
for (const MemRegion *R : Roots) {
State->scanReachableSymbols(loc::MemRegionVal(R), Visitor);
}
return Visitor.getState();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scanReachableSymbols is really expensive. Can't you invoke it only once and just pass Roots to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

return Visitor.getState();
}

ProgramStateRef getState() const { return State; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we no longer need this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


ProgramStateRef getState() const { return State; }

bool VisitSymbol(SymbolRef Sym) override {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be in the private part, if we made SymbolVisitor friend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved VisitSymbol to the private part of the class

Comment on lines 3123 to 3124
const auto *TST = QT->getAs<TemplateSpecializationType>();
if (TST) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const auto *TST = QT->getAs<TemplateSpecializationType>();
if (TST) {
if (const auto *TST = QT->getAs<TemplateSpecializationType>()) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied

Comment on lines 3220 to 3261
const Expr *AE = Call.getArgExpr(I);
if (!AE)
continue;
AE = AE->IgnoreParenImpCasts();

if (!isRvalueByValueRecordWithSmartPtr(AE))
continue;

// Find a region for the argument.
SVal VCall = Call.getArgSVal(I);
SVal VExpr = C.getSVal(AE);
const MemRegion *RCall = VCall.getAsRegion();
const MemRegion *RExpr = VExpr.getAsRegion();

const MemRegion *Base = RCall ? RCall : RExpr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

C.getSVal(Call.getArgExpr(I)) should be the same as Call.getArgSVal(I), so I don't understand why did you work so hard to get RCall and RExpr if they should be the same.
Do you have an example when RCall is different to RExpr?

Why do you call the result Base here? But from what I can see, nothing would imply that its a base region.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right - that one was fixed

Comment on lines 3235 to 3270
if (!Base) {
// Fallback: if we have a by-value record with smart pointer fields but no
// region, mark all allocated symbols as escaped
ProgramStateRef State = C.getState();
ProgramStateRef NewState = escapeAllAllocatedSymbols(State);
if (NewState != State)
C.addTransition(NewState);
continue;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a misuse of the addTransition API.
In this scenario you would want to chain the States across all of the arguments and only do a single transition.
This way the outcome wouldn't be what you expect. It would split the path N-ways, instead of having a single transition.
Also, you don't need the NewState != State. addTransition will do it for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refactored that part of the code and addressed the comment. Especially, I removed the comparison and just set needsStateUpdate = true when we know we need a state change

Comment on lines +70 to +78
void add(Foo foo) {
// The shared_ptr destructor will be called when foo goes out of scope
}

void test() {
// No warning should be emitted for this - the memory is managed by shared_ptr
// in the temporary Foo object, which will properly clean up the memory
add({make_shared<int>(1)});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have a similar test like this, but with multiple memory owning arguments. That will demonstrate the misuse of the addTransition API.
I would suggest you to have a look at the rewritten exploded graph of the case if you are interested.
clang/utils/analyzer/exploded-graph-rewriter.py
and -analyzer-dumpegraph=dump.dot

Comment on lines +34 to +42
void add(Foo foo) {
// The unique_ptr destructor will be called when foo goes out of scope
}

void test() {
// No warning should be emitted for this - the memory is managed by unique_ptr
// in the temporary Foo object, which will properly clean up the memory
add({make_unique<int>(1)});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also test this with multiple owning arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the test

…tests

- Applied requested refactoring in MallocChecker (API cleanup, code reuse, duplication reduction, base class field support).
- Merged unique_ptr and shared_ptr PR60896 tests into a single file.
@ivanmurashko ivanmurashko force-pushed the clang-analyzer/PR60896 branch from 97214cb to 333e5ba Compare August 9, 2025 19:25
Copy link
Contributor

@pskrgag pskrgag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work!

I am more C person, so I might be missing some CXX semantics. Except for nits left above patch LGTM.

- Applied minor style fixes and small improvements per review feedback.
@ivanmurashko ivanmurashko force-pushed the clang-analyzer/PR60896 branch from 7b26915 to 07cfed9 Compare August 9, 2025 20:58
- Moved duplicate "unique_ptr"/"shared_ptr" name check into a local lambda.
Consolidate multiple state transitions into single update to avoid path
splitting. Remove redundant state comparisons and simplify SVal handling.
Test multiple by-value arguments with smart pointer fields for
comprehensive coverage.
// Also try RecordType (for custom smart pointer implementations)
if (const auto *RD = QT->getAsCXXRecordDecl()) {
// Accept any custom unique_ptr or shared_ptr implementation
return (isSmartPtrName(RD->getName()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return (isSmartPtrName(RD->getName()));
return isSmartPtrName(RD->getName());

@pskrgag
Copy link
Contributor

pskrgag commented Aug 11, 2025

I am sorry, looks like I found another regression:

#include <memory>

struct Foo {
  std::unique_ptr<int> ptr;
  int *raw;

  Foo() : ptr(std::make_unique<int>(1)), raw(new int) {}
};

void add(Foo foo) {}

int main(int argc, const char **argv) {
  add(Foo());
  return 0;
}

With your patch applied raw leak is not reported, however current CSA finds it: https://godbolt.org/z/569x11e7a.

I didn't trace why it happens, will be able to come back tomorrow.

EDIT: If it would be too hard to solve, I guess we can live with it, since it very weird corner case. Beside that regression -- LGTM!

@ivanmurashko
Copy link
Contributor Author

I am sorry, looks like I found another regression:
...
With your patch applied raw leak is not reported, however current CSA finds it: https://godbolt.org/z/569x11e7a.

Thank you very much for the test case. I have to figure out the reason and fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:static analyzer clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Clang SA] Potential memory leak when init struct field during construction
4 participants